Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding cudf::get_current_device_resource_ref in place of rmm calls #2398

Open
wants to merge 4 commits into
base: branch-24.12
Choose a base branch
from

Conversation

hyperbolic2346
Copy link
Collaborator

This really closes #2358. It was closed earlier, but @harrism pointed out that we should wait for rapidsai/cudf#16679 and use the new cudf function. This is that update.

Copy link

@harrism harrism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually not rush this PR in. We are discovering issues with the Cython/C++ interface that are slowing the transition down. We have stepped back to do more codesign with the CCCL team to get this right. Things may change.

It's fine to merge this, just saying you may have to change it again later whether you merge this now or not. It will work with or without this change.

@@ -256,7 +256,7 @@ TEST_F(TimeZoneTest, ConvertFromUTCMicroseconds)
568036800000000L,
};
auto const actual = spark_rapids_jni::convert_utc_timestamp_to_timezone(
ts_col, *transitions, 1, cudf::get_default_stream(), rmm::mr::get_current_device_resource());
ts_col, *transitions, 1, cudf::get_default_stream(), cudf::get_current_device_resource_ref());

CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, *actual);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file needs a newline at the end (clang-format should be catching that).

@hyperbolic2346
Copy link
Collaborator Author

I would actually not rush this PR in. We are discovering issues with the Cython/C++ interface that are slowing the transition down. We have stepped back to do more codesign with the CCCL team to get this right. Things may change.

Do you have an issue or something to monitor for these issues and changes?

@harrism
Copy link

harrism commented Sep 18, 2024

Do you have an issue or something to monitor for these issues and changes?

rapidsai/rmm#1596

@harrism
Copy link

harrism commented Sep 18, 2024

Actually, the rest of libcudf has been updated to use cudf::get_current_device_resource_ref(), so there is no harm in merging this.

@sameerz
Copy link
Collaborator

sameerz commented Oct 11, 2024

@hyperbolic2346 can you please retarget to branch-24.12?

@hyperbolic2346 hyperbolic2346 changed the base branch from branch-24.10 to branch-24.12 October 16, 2024 15:34
@hyperbolic2346
Copy link
Collaborator Author

build

Copy link
Collaborator

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check before merging to see if there is any new places need to be modified, since the PR was updated a while ago.

@harrism
Copy link

harrism commented Oct 30, 2024

Please hold off. The design discussion on the CCCL side continues. I will let you know when it settles down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Move to RMM apis get_current_device_resource_ref
4 participants